Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking Datagram API change. #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Breaking Datagram API change. #25

wants to merge 2 commits into from

Conversation

cheatfate
Copy link
Collaborator

@cheatfate cheatfate commented Mar 24, 2019

DatagramCallback* = proc(transp: DatagramTransport,
                         remote: TransportAddress): Future[void] {.gcsafe.}
                         message: seq[byte],
                         remote: TransportAddress,
                         error: OSErrorCode): Future[void]

message - UDP message bytes.
If len(message) == 0 you need to check OSErrorCode to obtain error code.

@@ -28,7 +30,9 @@ type
writer: Future[void] # Writer vector completion Future

DatagramCallback* = proc(transp: DatagramTransport,
remote: TransportAddress): Future[void] {.gcsafe.}
message: seq[byte],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openArray[byte] tends to give more flexibility for the implementation.. a common option later on is to allow the original caller to set up a byte buffer of their choice..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also the kind of place where Result is appropriate - giving either an error or a buffer but never both - makes for cleaner code everywhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnetheduck we can't use openarray[T] because it can't be captured inside of iterator, so it can't work in async procedures.

remote: TransportAddress): Future[void] {.gcsafe.}
message: seq[byte],
remote: TransportAddress,
error: OSErrorCode): Future[void] {.gcsafe.}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like error which can happen here is either not realistic or unrelated to remote-specific message receiving. Thus I would consider removing the error from this function signature. For remote-unrelated events I would introduce another callback like onNetworkChanged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but chronos is cross-platform framework, and if Linux has small number of errors, BSD/MacOS and Windows has much more, so i wish to leave error code here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants